-
Notifications
You must be signed in to change notification settings - Fork 27k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Example: With cookie authentication #3955
Conversation
@timneutkens what is the review process? |
CC @sergiodxa, ... ? |
{user.email | ||
? ( | ||
<React.Fragment> | ||
<li onClick={processLogout} style={{ textDecoration: 'underline', cursor: 'pointer' }}>Logout</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A <button>
with a click handler should be used instead for proper semantics and better accessibility.
export const processLogin = async ({ email, password }) => { | ||
const user = await axios | ||
.post('/api/login', { email, password }) | ||
.then(response => response.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you're using async
, you can write this with async
:
const response = await axios.post('/api/login', { email, password })
const user = response.data
} | ||
} | ||
|
||
export const processLogout = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you can do this async
?
export const processLogout = async () => {
// ...
await axios.post('/api/logout')
Router.push('/login')
return { auth } | ||
} | ||
|
||
export const getProfile = () => axios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you can do this async
?
export const getProfile = async () => {
const response = await axios.get('/api/profile')
return response.data
}
I like this example. I was hoping to adapt it for auth0 but not sure where it is at. Any updates? |
I’ll update it. Not sure how the PR review / merge process works... i.e. who merges it to |
I think it has to go into canary first? Seems like @timneutkens is the one doing that regularly so maybe he will have feedback? |
Hmm... But what happens when I'll manually set |
@sarneeh still secure as the server looks for a signed cookie https://github.com/zeit/next.js/pull/3955/files#diff-59c49209a4ae9741eb402e16c3fbd1b4R61 |
@malixsys Yeah, my bad, didn't noticed this 😄 |
@timneutkens how does this get merged? |
|
||
// fake serverless authentication | ||
const authenticate = async (email, password) => { | ||
const users = await axios.get('https://jsonplaceholder.typicode.com/users').then(response => response.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using https cause error in development. Better use something like const scheme = dev ? 'http' : 'https'
?
return res.status(200).json(info) | ||
}) | ||
|
||
server.post('/api/logout', (req, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API should be separate from the Next.js server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be. On sites of a certain size, it probably should be.
This is an example. This way, there is no need to fire another server and no CORS problems...
|
||
server.use(express.json()) | ||
|
||
server.use(cookieParser(COOKIE_SECRET)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cookieparser shouldn't be used as it's an express middleware, instead use something like npmjs.com/next-cookies to parse the cookie in getInitialProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timneutkens Should the example be renamed?
This uses Express server-side to handle the Auth for demonstration purposes...
.then(() => { | ||
const server = express() | ||
|
||
server.use(express.json()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be used, I assume it won't be needed when moving out the API into it's own server.
|
||
const redirect = (res, path) => { | ||
if (res) { | ||
res.redirect(302, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.redirect
is not something officially supported, it comes from Express, this should use the location header.
const redirect = (res, path) => { | ||
if (res) { | ||
res.redirect(302, path) | ||
res.finished = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res.finished is no longer needed if res.end
is called
<Head /> | ||
<body> | ||
<Main /> | ||
<script dangerouslySetInnerHTML={{ __html: getUserScript(user) }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, you can use _app.js
and then assign to window.__USER__
in constructor of _app.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then _document.js can also be removed I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, no longer needed... This was written before _app I think... :)
@@ -0,0 +1,97 @@ | |||
import Router from 'next/router' | |||
import axios from 'axios' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use isomorphic-unfetch like the other examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this example is too opinionated to be included. 🤔
It works with Express and Axios because they are widespread and I find them easier to work with, and the reader might, too.
At the time of its creation in March, I could find no other comprehensive yet concise example of how to do proper secure, cookie-based auth...
Not bitter, just debating how willing I am to complexify this example with a separate API server.
Axios IS no biggy to fix.
@malixsys Invited you to the repository holding an auth example I started a while back: https://github.com/timneutkens/next.js-auth-example/invitations |
I'm going to close this PR in favor of #5821, thank you so much for your contribution though, I'm sure people have found this useful 🙏 |
No problem @timneutkens , I'll look at your invitation. |
This adds an example using cookies for authentication
c.f. #153